Skip to content

Ctpdev0#1012

Merged
knopers8 merged 14 commits into
AliceO2Group:masterfrom
mbombara:ctpdev0
Jan 21, 2022
Merged

Ctpdev0#1012
knopers8 merged 14 commits into
AliceO2Group:masterfrom
mbombara:ctpdev0

Conversation

@mbombara
Copy link
Copy Markdown
Contributor

Very first version of the QC CTP - the code can read raw data file and publish BC, trigger Inputs and trigger classes distributions.

Copy link
Copy Markdown
Collaborator

@Barthelemy Barthelemy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the new ctp module. Could you review my comments and make the changes ? Then I will re-review and approve.

Comment thread Modules/CTP/basic-ctp.json Outdated
"url": "infologger:///debug?qc"
},
"consul": {
"url": "http://consul-test.cern.ch:8500"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the address of consul.

Suggested change
"url": "http://consul-test.cern.ch:8500"
"url": ""

Comment thread Modules/CTP/basic-ctp.json
Comment thread Modules/CTP/basic-ctp.json Outdated
Comment thread Modules/CTP/basic-ctp.json Outdated
"id": "tst-raw",
"active": "true",
"machines": [],
"query": "random:TST/RAWDATA/0",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sending your data under TST/RAWDATA ? shouldn't it be under something like CTP/RAWDATA ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't sample data (yet) so I left it as it was from example. I am changing tst to ctp in both cases.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but if you are not sampling, what data are you monitoring ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This version is reading raw files locally - i.e. raw file is created from simulation (then digits, then raw file). I wanted to have it while waiting for interface by Sylvain. The payload structure will be the same in the real data so it is not waste of time. If I correctly understand, the data sampling is needed when you want monitor only fraction of data? here I monitor all (they are from simulation).

Comment thread Modules/CTP/src/RawDataQcCheck.cxx Outdated
@@ -0,0 +1,85 @@
// Copyright 2019-2020 CERN and copyright holders of ALICE O2.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that RawDataQcCheck is the default skeleton class. Please remove it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is. Do you mean removing RawDataQcCheck.cxx and RawDataQcCheck.h ?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, removing the whole class if you don't use it .

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also to remove the check in .json file?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and line in LinkDef.h?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes to all

Comment thread Modules/CTP/include/CTP/RawDataQcTask.h Outdated

/// \brief Example Quality Control DPL Task
/// \author My Name
class RawDataQcTask final : public TaskInterface
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please name the class in a more meaningful way.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would CTPRawDataReaderTask be better?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

Comment thread Modules/CTP/src/RawDataQcTask.cxx Outdated
ILOG(Info, Support) << "initialize RawDataQcTask" << ENDM; // QcInfoLogger is used. FairMQ logs will go to there as well.

// this is how to get access to custom parameters defined in the config file at qc.tasks.<task_name>.taskParameters
if (auto param = mCustomParameters.find("myOwnKey"); param != mCustomParameters.end()) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove example code

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is I do not know what is artefact and what is really needed. :-) I am removing lines 43 and 44.

Comment thread Modules/CTP/src/RawDataQcTask.cxx Outdated
getObjectsManager()->startPublishing(mHistoInputs);
getObjectsManager()->startPublishing(mHistoClasses);
try {
getObjectsManager()->addMetadata(mHistoBC->GetName(), "custom", "34");
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not necessary

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not know what those lines do, I thought they might be important. Is it ok to delete whole try { } catch { } section (lines 54-65)?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

Comment thread dpl-config.json Outdated
@@ -0,0 +1,18 @@
{
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this file needed ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea, I do not even know how it got there. I have removed it.

@Barthelemy
Copy link
Copy Markdown
Collaborator

do not forget to push your changes :)

@mbombara mbombara requested a review from njacazio as a code owner January 19, 2022 20:47
@knopers8 knopers8 merged commit b39c184 into AliceO2Group:master Jan 21, 2022
@mbombara mbombara deleted the ctpdev0 branch February 22, 2022 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants